rust: Channel and Schema structs have IDs #1297
Conversation
james-rms
force-pushed
the
jrms/channels-have-ids
branch
from
3c26630 to
5308c46
Compare
| @@ -20,28 +20,26 @@ pub struct Record { | |||
| } | |||
|
|
|||
| impl Record { | |||
| pub fn get_field(self: &Self, name: &str) -> &Value { | |||
There was a problem hiding this comment.
all changes in these files are due to the clippy --all-targets change.
|
cc. @mrkline |
| return Ok(*id); | ||
| // Adds a schema, returning its ID. If a schema with the same content has been added already, | ||
| // its ID is returned. | ||
| pub fn add_schema(&mut self, name: &str, encoding: &str, data: &[u8]) -> McapResult<u16> { |
There was a problem hiding this comment.
Thoughts on having a separate struct that's like "ChannelWithoutId"? Pros: avoid having to thread multiple params through different layers, avoid possible confusion between params of the same type.
|
|
||
| #[derive(Hash, PartialEq, Eq)] | ||
| struct SchemaContent<'a> { | ||
| name: Cow<'a, str>, |
There was a problem hiding this comment.
Could you ELI5 why we need Cow here?
There was a problem hiding this comment.
i'll add a comment. We use CoW because we want to compare equality between all of these values without neccessarily cloning or moving all of them into a struct to do that.
There was a problem hiding this comment.
Is there some overhead inherent in the Cow struct itself? Should I think of as being basically a union between a couple different storage options/types? or a pointer to an allocation elsewhere?
There was a problem hiding this comment.
a Cow is a union between a borrowed and owned version of a given type - so for str, it's (String | &str), for bytes it's (&[u8], Vec). It implements Deref for those types, allowing you to use their comparison, hash implementations against eachother.
In context, this struct lets me solves this problem:
struct left = {
a: String::new("abc"),
b: String::new("def")
}
struct right = {
a: "abc",
b: "def",
}
if a == b {
println!("they are the same!")
}
By defining the struct as having a, b be Cow<'a, str>`, I can store both owned and borrowed versions of the same type in it, and compare them against eachother.
| } | ||
| let next_schema_id = self.schemas.len() as u16 + 1; |
There was a problem hiding this comment.
Pedantic: We should probably return an error if there are more than 2^16-1 schemas.
| }) { | ||
| return Ok(id); | ||
| } | ||
| let next_channel_id = self.channels.len() as u16; |
There was a problem hiding this comment.
Pedantic: We should probably return an error if there are more than 2^16 channels.
Changelog
Breaking changes
idmembers to the crate::Channel and crate::Schema structs.Writer::add_channelinto two methods,Writer::add_schemaandWriter::add_channel.Docs
In docstrings, please review if not clear.
Description
When a message reader recieves schema and channel information, the schema and channel IDs in the MCAP are hidden from them. This PR adds those so that readers can observe ID information.
Additionally, this PR updates the Writer interface in these ways:
add_channel(Channel)method is replaced withadd_channel(schema_id, topic, message_encoding, metadata). This was done because the Channel struct now contains a channel ID, and to continue to use it would force the user to come up with their own channel ID before calling this function.add_schema(...)method is added to register schemas ahead of channels. This is needed to allow the caller to register a new schema without coming up with an ID in advance.write(Message)method is kept, but instead of automatically assigning new IDs for the message channel and schema, it uses the IDs passed in. This means that when writing functions that transform MCAP data, the IDs from the input can be kept in the output.